Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

making after hooks work even if the params argument was missing #20

Closed
wants to merge 1 commit into from

Conversation

ekryski
Copy link
Member

@ekryski ekryski commented Feb 5, 2015

This fixes #19. Still need to add a test for this

@daffl
Copy link
Member

daffl commented Feb 5, 2015

This is a good fix (with tests added) although having the parameters optional is turning out to be a big hassle . So many extra cases and bugs just so you don't have to type {},.

@ekryski
Copy link
Member Author

ekryski commented Feb 5, 2015

I know... :-(. I'm 50/50 on it right now. I'll have tests for this shortly. I actually need this fix asap.

@daffl
Copy link
Member

daffl commented Feb 5, 2015

If you're really that much against typing {}, 😉
Tests should be pretty easy once those are in I'll push a new release.

@daffl
Copy link
Member

daffl commented Feb 17, 2015

This is probably related to feathersjs/feathers#113. I think that if we always normalize and validate method parameters beforehand all hooks should work.

@ekryski
Copy link
Member Author

ekryski commented Feb 17, 2015

@daffl yes it seems to be causing a lot work arounds the fact that params can be optional. We should make it required that you pass at least {}.

@daffl
Copy link
Member

daffl commented Feb 17, 2015

I agree. As @marshallswain pointed out though, it is still easy to break the server by sending the wrong arguments through the socket.

@ekryski
Copy link
Member Author

ekryski commented Feb 17, 2015

Ya I saw that 😢. Let's make it required and then we can emit nice errors telling people that they need to at least pass {}. I'm now caught up on sleep and looking to do some coding this afternoon/evening.

@ekryski
Copy link
Member Author

ekryski commented Mar 6, 2015

We should actually use the argument normalization from feathers-commons and perform argument normalization on this line

@daffl
Copy link
Member

daffl commented Oct 6, 2015

This has been done and is released in version 1.1.0.

@daffl daffl closed this Oct 6, 2015
@daffl daffl deleted the issue-#19 branch October 6, 2015 02:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After hooks fail if params was missing on the original service method
2 participants